feat(query-builder): support for nested queries and other improvements #14647
Conversation
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
igdmdimitrov
added
the
🛠️ status: in-development
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
The '+' icons in the add "'and'/'or' group" buttons are not displayed. Suggestion: move the registerSVGIcons method to the query-builder.component.ts and call it on initial loading because when there is no expression tree a query-builder-tree is not created and the method is not called |
Should we keep the previous selection? |
Result: Should we open the dialog when initially setting (not changing) the entity? |
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
|
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
| @@ -15,6 +16,8 @@ const QueryBuilderResourceStringsIT_: ExpandRequire<IQueryBuilderResourceStrings | |||
| igx_query_builder_filter_notEmpty: 'Non vuoto', | |||
| igx_query_builder_filter_null: 'Null', | |||
| igx_query_builder_filter_notNull: 'Non null', | |||
| igx_query_builder_filter_in: 'In', | |||
| @@ -15,6 +16,8 @@ const QueryBuilderResourceStringsKO_: ExpandRequire<IQueryBuilderResourceStrings | |||
| igx_query_builder_filter_notEmpty: '비우지 않음', | |||
| igx_query_builder_filter_null: 'Null', | |||
| igx_query_builder_filter_notNull: 'Not Null', | |||
| igx_query_builder_filter_in: 'In', | |||
| this.setDragCursor('grab'); | ||
|
|
||
| //TODO z-index is set, but ghost still not visible in Dialog | ||
| if(this.dragGhostElement.style) this.dragGhostElement.style.zIndex = "9999"; |
There was a problem hiding this comment.
DRAG GHOST NOT VISIBLE IN DIALOG
Event after setting the z-index of the drag ghost to 9999, it's still not visible over the dialog.
As you can see, the ghost is rendered as last child of the body, outside app-root and overlay and we don't have control over this:
There was a problem hiding this comment.
Seems that the overlay is with z-index 10005.
That's why 9999was not working. 10006 is...
There was a problem hiding this comment.
@ivanvpetrov Why can't move the ghost element inside the query builder?
Adding a z-index to the ghost element is not a viable solution, as any future changes to the z-index of the overlay could result in unintended regressions.
There was a problem hiding this comment.
We have almost no control over the drag ghost. It's part of the igx-chip's drag directive and we use it out of the box.
I've looked at other examples along the documentation, the drag ghost is always a sibling of the body, outside any enclosure.
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
projects/igniteui-angular/src/lib/query-builder/query-builder.component.spec.ts
Outdated
Show resolved
Hide resolved
projects/igniteui-angular/src/lib/query-builder/query-builder.component.spec.ts
Outdated
Show resolved
Hide resolved
…component.spec.ts Co-authored-by: Galina Edinakova <[email protected]>
…component.spec.ts Co-authored-by: Galina Edinakova <[email protected]>
projects/igniteui-angular/src/lib/query-builder/query-builder.component.spec.ts
Outdated
Show resolved
Hide resolved
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
* feat(query-builder): focus on droped chip * test(query-builder): add focus on drop tests --------- Co-authored-by: Galina Edinakova <[email protected]>
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
|
Indigo
|
There was a problem hiding this comment.
Shouldn't these be 19.1 migrations?
| @@ -131,7 +131,7 @@ export class IgxAdvancedFilteringDialogComponent implements AfterViewInit, OnDes | |||
| eventArgs.stopPropagation(); | |||
| const key = eventArgs.key; | |||
| if (this.queryBuilder.isContextMenuVisible && (key === this.platform.KEYMAP.ESCAPE)) { | |||
| this.queryBuilder.clearSelection(); | |||
|
|
|||
There was a problem hiding this comment.
Looks like an empty if block?
| queryBuilder.expressionTree = QueryBuilderFunctions.generateExpressionTree(); | ||
| fix.detectChanges(); | ||
| tick(100); | ||
| fix.detectChanges(); |
There was a problem hiding this comment.
Generally speaking we are trying to avoid arbitrarily timed ticks
Also
fix.detectChanges();
tick(100);
fix.detectChanges();
This is a doom sandwich :) Why is it needed?
There was a problem hiding this comment.
Based on the changelog, these are not the only changes that the ReadMe should receive
There was a problem hiding this comment.
It's probably not the only file that has these but I notice quite a lot of == in conditions. As part of the best practices we try to employ we use == only when entirely intentional as it's erroneous in JS.
e.g. you have::
this._lastFocusedChipIndex == undefined ? ...
which actually is true if lastFocusedChipIndex is null. Unless this is intentional just use === as it makes the file more easy to understand on the fly as maintainers don't have to think if you intended for the == or not.
|
|
||
| public onReturnFieldSelectChanging(event: IComboSelectionChangingEventArgs | ISelectionEventArgs) { | ||
| let newSelection = []; | ||
| if (event.newSelection instanceof Array) { |
There was a problem hiding this comment.
Not the biggest fan of instanceof when it can be avoided. It probably wouldn't matter in this case but have in mind that it can be erroneous . As there is Array.isArray method, you don't have to use it here.
| return groupItem; | ||
| } | ||
|
|
||
| for (const expr of expressionTree.filteringOperands) { |
There was a problem hiding this comment.
for of loops look cool but they are a lot ot slower than traditional for i; i< array.length . Sometimes traditional is better :)
|
|
||
| //Get the dragged ghost as a HTMLElement | ||
| private get dragGhostElement(): HTMLElement { | ||
| return (document.querySelector('.igx-chip__ghost[ghostclass="igx-chip__ghost"]') as HTMLElement); |
There was a problem hiding this comment.
Have you checked what happens if you have two QBs on the page?
|
|
||
| //Create the drop ghost node based on the base chip that's been dragged | ||
| private createDropGhost(keyboardMode?: boolean) { | ||
| const dragCopy = this.sourceElement.cloneNode(true); |
There was a problem hiding this comment.
This is DOM magic galore. Isn't there a way to template the ghost instead?
| private renderDropGhostChip(appendToElement: HTMLElement, appendUnder: boolean, keyboardMode?: boolean): void { | ||
| const dragCopy = this.createDropGhost(keyboardMode); | ||
|
|
||
| //Append the ghost |
There was a problem hiding this comment.
See above. We want to minimize DOM manipulation in Angulara as much as possible. I am not going to stop the pr from being merged because of this but double check if it's possible to tempalte this please.
|
Keyboard interactions via tab and shift+tab stop working.
|
This behavior is made intentionally. Once you start the drag&drop procedure (hance hitting up/down) the only way to exit this state is confirming(Enter) or canceling(Esc). That's why Tabs is inhibited. |
…b.com/IgniteUI/igniteui-angular into dmdimitrov/query-builder-improvements
Closes #14642
Closes #14979
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)